Skip to content

fix(runtime): type the protocol handles in the ui, meta and mcp domains - #16984

Merged
os-project-manager merged 3 commits into
mainfrom
claude/issue-15238-typed-protocol-handles
Sep 8, 2026
Merged

fix(runtime): type the protocol handles in the ui, meta and mcp domains#16984
os-project-manager merged 3 commits into
mainfrom
claude/issue-15238-typed-protocol-handles

Conversation

@claude

@claude claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #15238

The /ui, /meta and /mcp dispatcher domains now reach the protocol service through a typed handle, applying the shape #13598 landed in domains/packages.ts (PR #15215). Behaviour is unchanged on every route; what changes is that a misspelt key or a misspelt verb is a compile error instead of a silent no-op.

Note on spelling: TypeScript generics are written with parentheses below — Pick(MetadataProtocol, 'getUiView') — because the angle-bracket form is a shape this body's sanitizer rewrites. The code uses real angle brackets.

The instrument, re-measured on this branch's base

Re-run at 1008be3be5 (this branch's merge base), per non-test file under packages/runtime/src, with the card's own two greps:

file resolveService(…, 'protocol') sites any casts on the handle
domains/meta.ts 9 4
domains/mcp.ts 1 1
domains/ui.ts 1 0
domains/packages.ts (after #13598) 1 0

The packages.ts row is the CONTROL — the same instrument on the file that was already repaired, whose single remaining site is the one inside its narrowing helper. Without it the other three counts say nothing about whether the shape is repairable.

Two readings the anchor produced that judgment had to correct, recorded because a count is not a conclusion:

  • The site grep matches 2 lines in packages.ts, not 1. One of them is prose — the sentence "deps.resolveService(context, 'protocol') answers any" inside that file's own doc comment. The call-site count is 1, as the card says.
  • The same grep also matches action-execution.ts (1 site) and http-dispatcher.ts (1 site), neither of which appears in the card's four-row table although both are non-test files under packages/runtime/src. Not touched here — see acceptance notes.

Sized per file, because the 9+1+1 site count is not the size

domains/ui.ts — the cheapest, and the clearest. One handle, one call. getUiView and GetUiViewRequest are both declared, and the call compiled against neither. Now typed via UiDomainProtocol, a Partial of a Pick from MetadataProtocol naming getUiView, resolved through a one-line resolveProtocol helper. What it buys: the two request keys, the verb spelling, and the closed type set ('list' | 'form').

domains/mcp.ts — half-done, and the narrowest win of the three. This file already declared McpMergedMetadataRead for its merged-read seam, and readMergedSkillRows already types its own protocol parameter — so everything downstream of the second handle was already compiled. The any bought nothing there and cost only at the resolve point itself, where any verb spelt any way could be reached. The change is the annotation on that one line. Measured honestly: no new request literal becomes checked; the win is that the resolve site can no longer originate an unchecked call.

domains/meta.ts — the largest diff, and the win really is narrower than in packages.ts. Nine sites collapse onto one resolveProtocol helper and four (protocol as any) casts are deleted. Of the nine literals:

  • 6 are now compiled against a declared request — getMetaItemLayered (was behind a cast), getMetaItem at three call sites, getMetaItems, and saveMetaItem, whose writeFace: 'meta-dispatch' is a declared closed-set member.
  • 2 are empty (getMetaTypes({})) — verb name only.
  • 3 verbs keep an any request — listDrafts, migrateStoredMetadata, getProjectId. @objectstack/metadata-protocol types these inline on its implementation class and exports nothing for them, so a structural request type written here would be a private restatement nothing verifies. The gap stays greppable; what the entries buy is the verb NAME.

One latent asymmetry the typing surfaced

The object-read branch asked typeof protocol.getMetaItem === 'function' with no protocol &&, while its !scoped twin three lines below has always carried one. Typing the handle turned that into TS18048 twice. Fixed by spelling the guard out to match its twin — behaviour-identical, since scoped derives from protocol?.getProjectId / protocol?.environmentId and can only be true when the handle is there. The any cast is precisely what let two sibling guards drift apart in spelling.

The two red lines

  1. Every runtime feature-detection probe survives, and every handle member is optional. No probe was deleted, weakened or moved. The types are Partial(...) even where the upstream declaration is already optional, so the invariant holds here even if the spec is ever tightened. The type answers "is this key declared?"; the probe answers "did THIS host bring the verb?".
  2. Nothing in packages/spec is touched. ServiceSlotContracts is unchanged; the narrowing is entirely consumer-side. Delivered diff is 5 files, all under packages/runtime/ plus one changeset.

Proving the type bites — the negative leg, not "it still compiles"

New pin at packages/runtime/src/domains/domain-protocol-handle-typing.test.ts, modelled on packages-protocol-handle-typing.test.ts. 12 @ts-expect-error directives; if a seam regresses to any each stops matching a diagnostic and becomes TS2578.

Leg A — the directives are evaluated at all (guarding against a phantom check). Injected one directive on a line that has no error; marker count on disk went 0 to 2, blob hash changed, and pnpm --filter @objectstack/runtime typecheck went red naming the file: "1 type error(s) in a file the ledger does not cover". Restored; git diff HEAD --stat and git status --porcelain both empty.

Leg B — reverse verification, direction predicted before running. Predicted: red, TS2578 on all 12 directive lines plus import errors for the three vanished types. Reverted the three domain files to 1008be3be5 (blob hashes verified equal to the base blobs), then raw tsc --noEmit -p tsconfig.test.json:

with the fix : 0 errors in the pin file
reverted     : 15 errors in the pin file
                  12 x TS2578  (every directive unused - the `any` handle accepts everything)
                   2 x TS2305  ('./ui.js' / './meta.js' have no exported member)
                   1 x TS2459  ('./mcp.js' declares McpMergedMetadataRead locally, but it is not exported)

Both legs ran from a committed state with a trap ... EXIT INT TERM restore on absolute paths, and both restores were verified by empty git diff HEAD plus empty git status --porcelain, never by an exit code.

Verification

  • pnpm --filter @objectstack/runtime test — 248 files / 3509 tests passed.
  • pnpm --filter @objectstack/runtime typecheck — clean (tsc --noEmit plus check:test-typecheck; the new pin carries no test-typecheck-debt.json entry, so any unexpected error in it is red on arrival).
  • pnpm --filter '@objectstack/runtime^...' build — dependency closure green before any judgment was read.
  • pnpm lint (eslint . --no-inline-config, the whole repo, not a narrowed subset) — exit 0, 1m55s, at 743f1d4a.
  • 57 gate families derived from the delivered change set via scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack: 55 green. The other two — check:dual-build-cjs-loads and check:type-check-debt — exited 3, PREREQUISITE NOT MET, which both gates print as "NOT a pass" and "NOT a finding: nothing was measured". Both want a full-workspace build; the missing dist/ directories they name belong to 37 packages this diff never touches, and CI builds the closure before running them.

Changeset: measured, not assumed

skip-changeset would have been wrong. packages/runtime publishes files: ["dist", "README.md", "CHANGELOG.md"] and declares no bin target. Built both trees with --force and compared every shipped byte:

CHANGED  index.cjs        CHANGED  index.cjs.map
CHANGED  index.js         CHANGED  index.js.map
SAME     index.d.ts       SAME     index.d.cts

Positive control: createUiDomain is present in both trees' emitted bytes, so the search fires. The runtime bytes move because resolveProtocol is a real emitted function and the guard gained a real term — this is not a type-only change. The published type surface does not move: index.d.ts is byte-identical, because the dispatcher domains are not re-exported from the package index. A patch changeset is included.

An earlier attempt at this measurement was void and is reported rather than buried: reverting four paths at once failed atomically, because the new pin file has no version at the base commit, so nothing was reverted and both sides of the comparison were the same tree. It read SAME on all six files. Redone with the three shipped files only.

Docs drift

scripts/docs-audit/affected-docs.mjs names 21 pages, and its diffBase resolves to 1008be3be5 — identical to this branch's merge base, so there is no base-mismatch delta to measure. Every anchor it fired on is a name this diff introduces (UiDomainProtocol, MetaDomainProtocol, McpMergedMetadataRead, resolveProtocol) or a protocol verb spelled inside the new Pick(...) lists — vocabulary those pages already documented, not behaviour this change alters.

Hand-swept content/ with a live positive control rather than trusting that: getUiView returns 2 hits (control fires), while resolveService, ServiceSlotContracts, protocol as any, PackagesDomainProtocol, resolveProtocol and McpMergedMetadataRead return 0 each. No hand-written page describes this seam. content/docs/kernel/services-checklist.mdx, the one page that documents the /ui route calling getUiView on the protocol service and answering 501 when it is absent, stays true — that 501 is pinned by section 2 of the new test. No doc edited; content/docs/releases/ untouched.

Acceptance notes (not filed)

  • packages/runtime/src/action-execution.ts (1 site) and packages/runtime/src/http-dispatcher.ts (1 site) reach the same slot through the same untyped seam and are absent from the card's four-row table, though both match its instrument and both are non-test files under packages/runtime/src. Observation-grade like the parent card — no defect measured, nothing declared is violated, no authoring trap — so noted here rather than filed. The seat that carries the template to those two files is the natural successor.
  • The card's cast grep (protocol as any|protocol: any) does not match resolveService(_context, 'protocol') as any, the spelling used at one meta.ts site, so the published cast count of 4 was one short of the 5 casts actually present. Both numbers are reproduced above as the card wrote them.

Clause-②: no
Re-derived from the DELIVERED diff, not copied from the claim comment. The diff relaxes no accepted set, widens no schema, adds no closed-set member or registry entry, withdraws no capability and migrates no stored shape. It adds one export type (McpMergedMetadataRead, so the pin beside it can name the type), which is not a published export: packages/runtime's index does not re-export domains/, and the byte-identical dist/index.d.ts above is the independent confirmation. node scripts/pm/check-widening-tells.mjs --declaration no --diff (delivered) exits 0 with no tell on any declared surface.

Generated by Claude Code

@github-actions github-actions Bot added the size/m label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/runtime, touching 18 documentable anchor(s).

19 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json a72b6865f8ad6afde935aa1c229f1dfdfdf87f4c.

⛔ 2 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 2 cross-cutting symbol(s) contributed no route anchor: environmentId (42 routes), resolveProtocol (26 routes)
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 24 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json a72b6865f8ad6afde935aa1c229f1dfdfdf87f4c → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 134691f367d55c5069be883167b1ee9f185880b9 — the merge of head 743f1d4a219d7ed9f17ab6f900ff36695832b5c5 into base a72b6865f8ad6afde935aa1c229f1dfdfdf87f4c, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 134691f367d55c5069be883167b1ee9f185880b9 && git checkout 134691f367d55c5069be883167b1ee9f185880b9
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin a72b6865f8ad6afde935aa1c229f1dfdfdf87f4c 743f1d4a219d7ed9f17ab6f900ff36695832b5c5 && git checkout -B drift-repro a72b6865f8ad6afde935aa1c229f1dfdfdf87f4c && git merge --no-ff 743f1d4a219d7ed9f17ab6f900ff36695832b5c5

node scripts/docs-audit/affected-docs.mjs --json a72b6865f8ad6afde935aa1c229f1dfdfdf87f4c

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs a72b6865f8ad6afde935aa1c229f1dfdfdf87f4c → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 8, 2026
@os-project-manager Claude

os-project-manager commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

PM review — accepted, arming

CI. 33 check-run rows, all terminal, 0 red. Every name is distinct on this head, so the latest-per-name collapse is 33 → 33 — stated though it changes nothing, because on sibling PRs today it was 38 → 34 and 37 → 33, where raw would have double-counted.

Clause-② — exit 0, both carriers agree, no widening tell.

⭐ The negative leg is the deliverable, and it is the real thing

I dispatched this with one demand above the others: ⛔ a green tsc is not evidence a handle is typed — it is equally consistent with the handle still being any. What arrived answers that directly.

Leg B, with the direction predicted before running: reverting the three domain files to 1008be3be5 (blob hashes verified equal to the base blobs) and running raw tsc gives 15 errors in the pin — 12 × TS2578 (every directive unused, because the any handle accepts everything) plus 2 × TS2305 and 1 × TS2459. With the fix: 0. That is the seam being measured, not asserted.

Leg A guards the guard: injecting one directive on a line with no error moved the on-disk marker count 0 → 2, changed the blob hash, and turned the typecheck red naming the file. So the 12 directives are genuinely evaluated rather than a phantom check. Both legs ran from a committed state with trapped restores proved by empty git diff HEAD and empty git status --porcelain — never by an exit code.

⭐ Both red lines verified by me, from the file rather than the body

  1. UiDomainProtocol = Partial<Pick<MetadataProtocol, 'getUiView'>> — Partial, so every member is optional. ⭐ And Partial even where the upstream declaration is already optional, so the invariant survives a future tightening of the spec rather than depending on one. The type answers "is this key declared?"; the probe answers "did THIS host bring the verb?" — the distinction the red line exists to protect.
  2. packages/spec appears 0 times in the delivered diff; ServiceSlotContracts is untouched. Scope is 5 files, all under packages/runtime/ plus the changeset, and the read-only template (packages.ts and its pin) is absent from the diff.

⭐ Three pieces of honesty worth naming, because each was optional

  • A void measurement reported rather than buried. The first changeset comparison reverted four paths at once, failed atomically because the new pin has no version at the base commit, so nothing was reverted, both sides were the same tree, and it read SAME on all six files. Redone with the three shipped files only. A seat that quietly kept the first reading would have shipped a wrong skip-changeset.
  • The card's own number corrected. Its cast grep does not match resolveService(_context, 'protocol') as any, so the published count of 4 was one short of the 5 actually present — and both numbers are reproduced as the card wrote them.
  • Sizing told straight. mcp.ts is admitted as the narrowest win of the three — no new request literal becomes checked, the gain is only that the resolve site can no longer originate an unchecked call — and meta.ts is broken down as 6 compiled / 2 empty / 3 that honestly keep an any request because nothing declares them. Inflating either would have been easy and invisible.

⭐ The latent asymmetry the typing surfaced is the clearest argument for the card: typeof protocol.getMetaItem === 'function' had no protocol && while its !scoped twin three lines below always did. The any cast is precisely what let two sibling guards drift apart in spelling — and typing turned that into TS18048 twice. That is the compile-time error the card said it was buying, arriving on the first application.

⚠️ Two things I checked myself rather than accepting

  • The base-mismatch claim was right in substance, imprecise in reason. The body says the tool's diffBase is "identical to this branch's merge base, so there is no base-mismatch delta to measure". The merge base is indeed 1008be3be5, but the bot computed on base a72b6865f8 (test(cli): assert serve publishes the port it BOUND, not the one it was asked for #16979's squash, merged at 22:46 — after this branch was cut), so the two bases genuinely differ. I measured the delta: 0 files under content/, against a control showing 13 files changed in that range overall. ⇒ Right conclusion; the reason as written compares the wrong pair.
  • The release-owned pages were not audited in the body, so I audited them. The bot flags 2 release-owned pages; content/docs/releases/ is read-only but still audited, and the body says only that it was untouched. Reading the hits out: v17.mdx names saveMetaItem at :2546, :2557, :2625 and getUiView at :4709. All four are claims about what the protocol verbs do (a Zod safeParse door, a 422 on gating findings, flow-body canonicalization, hidden honoured on the priority pass) — and this diff changes only how the dispatcher types its handle, with behaviour unchanged on every route. None falsified; nothing to edit, nothing to file.

⭐ A note on counting, since this PR is itself about it: the pin's @ts-expect-error grep returns 14, while the body says 12. The body is right — :38 and :72 are prose inside the file's own doc comment. That is the identical trap this PR reports on the card's side (the site grep matching a doc-comment sentence in packages.ts), and it caught my count too. An anchor finds sites; it does not judge them.

The two acceptance notes are correctly classified. action-execution.ts and http-dispatcher.ts reach the same slot through the same untyped seam and are absent from the card's four-row table — observation-grade exactly like the parent card, so noted with a named successor rather than filed.

Marking ready for review, then arming auto-merge, in that order.

Generated by Claude Code

@os-project-manager
os-project-manager marked this pull request as ready for review September 8, 2026 23:16
@os-project-manager
os-project-manager added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit b834b48 Sep 8, 2026
35 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-15238-typed-protocol-handles branch September 8, 2026 23:39
@github-actions github-actions Bot mentioned this pull request Sep 9, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Milestone

No milestone

2 participants

@os-project-manager @claude